-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(geocore): fixed errors with sublayers showing as loading endlessly #2700
fix(geocore): fixed errors with sublayers showing as loading endlessly #2700
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @DamonU2)
packages/geoview-core/src/geo/layer/layer.ts
line 709 at r1 (raw file):
const legendLayerInfo = LegendEventProcessor.getLegendLayerInfo(this.getMapId(), payload.layerPath); // Ensure that the layer bounds and legend icons are set when the layer is loaded if (legendLayerInfo && !legendLayerInfo.bounds) LegendEventProcessor.getLayerBounds(this.getMapId(), payload.layerPath);
If bounds are not set but icons is, would it pass the function without triggering a logError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @jolevesq)
packages/geoview-core/src/geo/layer/layer.ts
line 709 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
If bounds are not set but icons is, would it pass the function without triggering a logError?
Yes, the catch is just there because it is required for an async function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/geo/layer/layer.ts
line 716 at r1 (raw file):
}); this.#emitLayerLoaded({ layer: sender, layerPath: payload.layerPath });
Question: As it is, the emitLayerLoaded
will be emitted before the queryLegend()
has terminated. I don't remember if that's alright or not (based on the layer statuses). So, basically asking if that's what we want. If so, all good. If not, maybe we're missing an await
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/geo/layer/layer.ts
line 716 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Question: As it is, the
emitLayerLoaded
will be emitted before thequeryLegend()
has terminated. I don't remember if that's alright or not (based on the layer statuses). So, basically asking if that's what we want. If so, all good. If not, maybe we're missing anawait
here.
Reading the code again, it probably aligns with how it was before.
Therefore, my question should rather be, is the queryLegend() in LegendsLayerSet
not good enough to do this over there instead of here? I'm likely missing some context explaining this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)
packages/geoview-core/src/geo/layer/layer.ts
line 716 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Reading the code again, it probably aligns with how it was before.
Therefore, my question should rather be, is the queryLegend() inLegendsLayerSet
not good enough to do this over there instead of here? I'm likely missing some context explaining this change.
Yeah, looking a little deeper, this probably isn't helping. We're having issues with some layers querying the legend before the style is set, and not updating once the style is there, but this doesn't fix that. Removed and I'll look elsewhere.
ea5922c
to
e25b7f6
Compare
Also fixes issues with legend not loading Closes Canadian-Geospatial-Platform#2689
e25b7f6
to
55b5eaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)
7142523
into
Canadian-Geospatial-Platform:develop
Also fixes issues with legend not loading
Closes #2689
Description
Fixes issues with invalid extent from the metadata causing issues. Also checks for legend icons and bounds once layer is loaded.
Type of change
How Has This Been Tested?
https://damonu2.github.io/geoview/add-layers.html
5a65ad7c-561a-466a-8375-8d876624df9d
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is